Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of succ & fail marks on rate summary #3765

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

joanlopez
Copy link
Contributor

What?

It replaces the use of succ & fail marks (✓/✗) for rate metrics on summary, in favor of the text: {success} out of {total}.

Note: the check summary remains the same, as for checks there's no ambiguity on the use of ✓ and ✗.

Why?

Because as discussed in #2306 and #3656, the current form is sometimes confusing, because the metric may have either a "positive" meaning (like successful requests) or a "negative" meaning (like failed requests), for which the use of ✓ may result in confusions, especially for the latter.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #2306

Second iteration of #3656, based on what was discussed there.

@joanlopez joanlopez self-assigned this Jun 4, 2024
@joanlopez joanlopez requested a review from a team as a code owner June 4, 2024 12:22
@joanlopez joanlopez modified the milestones: v0.52.0, v0.53.0 Jun 17, 2024
@joanlopez joanlopez modified the milestones: v0.53.0, v0.55.0 Jul 24, 2024
@oleiade
Copy link
Member

oleiade commented Aug 22, 2024

This PR aligns with our goal and designs to refactor the end-of-test summary. As a matter of fact, we integrated its approach in our designs. From my perspective, this is good to go and merge, in principle 👍🏻

@olegbespalov olegbespalov modified the milestones: v0.55.0, v0.54.0 Sep 12, 2024
@joanlopez
Copy link
Contributor Author

joanlopez commented Sep 12, 2024

We (@oleiade and I, among others) have been discussing details like what's mentioned in #3765 (comment), and others covered in the design doc, like grouping metrics (http, network, execution, etc) and we agreed to merge this, as it already brings some value to those users that still nowadays are getting confused with the current format using ✓ and ✗ for rate metrics (see this example), and is compatible with changes that will come in the near future.

So, we ship some value in the next/current release, and iterate later, instead of waiting till the end result to ship the value.

Besides, I'm not adding the breaking-change label, as we don't consider this summary format to be part of the expectations associated with version compatibility, while others could (e.g. machine readable ones, like JSON).

@joanlopez joanlopez merged commit 910eb0d into grafana:master Sep 12, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Console output] http_req_failed : "succMark" & "failMark" seems inversed?
5 participants